-
Notifications
You must be signed in to change notification settings - Fork 26
Indicating when the body wasn't stored for the audited message #2585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f95b9c0
to
293120f
Compare
<div v-if="bodyState.not_found" class="alert alert-info">Could not find the message body. This could be because the message URL is invalid or the corresponding message was processed and is no longer tracked by ServiceControl.</div> | ||
<div v-else-if="bodyState.failed_to_load" class="alert alert-info">Message body unavailable.</div> | ||
<LoadingSpinner v-else-if="bodyState.loading" /> | ||
<div v-else-if="body === ''" class="alert alert-info">Body was too large and not stored. Edit ServiceControl/MaxBodySizeToStore to be larger in the ServiceControl configuration.</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion - it will be good to link to the doc as well
https://docs.particular.net/servicecontrol/audit-instances/configuration#performance-tuning-servicecontrol-auditmaxbodysizetostore
Also not a great fan of checking for empty . We could tie to a new empty state in 'bodystate'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed both. Please let me know if that looks better.
a4d2220
to
5fb8f03
Compare
5fb8f03
to
44ac18a
Compare
@danielmarbach Thank you. Your approach is much better. I took it into my branch. Regarding sagas: I can't reproduce the behavior of "empty body" there. ServicePulse happily showed the saga state with 10MB data. When I increased it to 100MB, the audit instance broke and couldn't retrieve the message from RabbitMQ. I'm not sure it would drop the too big saga state. Are you sure it happens (and where)? |
@afprtclr that's interesting. I pushed a commit showing the path that reaches to download the message body from the same endpoint URL, if I'm not mistaken, and therefore should also deal with the 204 returned. I haven't had a chance to wrap my head around the specs but maybe we can also add one or two specs to both code paths |
@danielmarbach I can merge your proposal, but I'm not sure we should add it if 204 is never returned for saga (which would render the code redundant). |
For: